Murdock results

✔️ PASSED

a8ccd54 tests/unittests: Add unittest for CRC8-LSB

Artifacts

• Documentation preview

The latest fixup adds a dedicated test with the Onewire parameters, because why not.

We should probably consider renaming the _lsb() to _reflected(), but I like not mixing that concern with adding the implementation and keeping the naming convention.

But also, the reflected version does operate least significant bit first, so the _lsb() naming does have some merit.

We should probably consider renaming the _lsb() to _reflected(), but I like not mixing that concern with adding the implementation and keeping the naming convention.

But also, the reflected version does operate least significant bit first, so the _lsb() naming does have some merit.

There is some value in having that discussion before merging, because afterwards, once a Release has happened, we would have to deprecate the old function name if we decided to change it.

However I'm a bit torn, because the new function does not reflect the data, it just works LSB first (which is equivalent to reflecting the data beforehand). But on the other side, Maxim doesn't really call that algorithm "reflected".
Internally everything just works LSB first and therefore also the CRC works LSB first.

There is some value in having that discussion before merging

Could it be that you are not yet aware of how much energy the RIOT community has for bike-shedding and that discussions about naming easily take many months? 🤣

If we were to change the name, we would have to add a backward compatible alias for the CRC16 variant anyway. I think it would not make much difference to do the same for CRC8.

I'm personally fine with either name, especially as the doc here states both terminologies, so that people will find the function they are looking for easily.

Well then let's get this merged so I can get back to poking @Enoch247 :)